Tests are a bit unhappy. I always twitch when I see anything to do with symlinks. I can't point a finger at anything wrong with this, but I'm not sure if it's a good idea to do the copy of the symlink.

@flouthoc ?

I can't point a finger at anything wrong with this, but I'm not sure if it's a good idea to do the copy of the symlink.

You're right, but unfortunately, that's how docker does things right now. They cp the symlink as it is irrespective of whether the target is present or not. I'll try to look for what's the rationale behind this but otherwise, this should be harmless with a global bool @rhatdan suggested. Wdyt?

If the cp is supposed to be into the container image, then we need to make sure the symbolic link still points within the image. IE Make sure it does not point at the hosts /etc/passwd.

Similarly if you are copying from the container to the host, we should make sure the source is within the container image.

then we need to make sure the symbolic link still points within the image.

Are you suggesting we move the target to the container and then create a symbolic link? That'd be quite surprising in terms of user experience, wouldn't it? And besides, it'll be diverting quite far from how docker does things.

IE Make sure it does not point at the hosts /etc/passwd.

With this change, it would point to the host's /etc/passwd but it would just be an empty link anyways.

Ok this LGTM, now we need to fix tests.

A friendly reminder that this PR had no activity for 30 days.

Planning to add a test for this over at containers/podman#18020 utilizing the --follow-link flag. I tried doing it here, but we don't plan to add the flag here and copier would anyway just return the link I create as part of the unit tests. Thoughts, @rhatdan?

A friendly reminder that this PR had no activity for 30 days.

LGTM
@flouthoc PTAL

@flouthoc @nalind PTAL

Without a unit test, I'm very reluctant to change things in copier.

Without changing the info value that's passed to copierHandlerGetOne(), the Typeflag in the header it outputs for the tar archive shouldn't be changed, so I'm not sure this produces the desired effect. But this does point out that we weren't reading the target info for the symlink when the loop at line 1160 wasn't being entered, and that's definitely a bug. It would probably make more sense to add the os.Readlink call closer to there, though.

A friendly reminder that this PR had no activity for 30 days.

@danishprakash any progress with the test?

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: danishprakash
Once this PR has been reviewed and has the lgtm label, please assign rhatdan for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Without changing the info value that's passed to copierHandlerGetOne(), the Typeflag in the header it outputs for the tar archive shouldn't be changed, so I'm not sure this produces the desired effect

From what I know, the problem isn't with symlinks but with how we deal with the target when not following links. So If I understand the issue, this is what's happening:

During the copy from the host, Get() was always following the links before creating the archive[1]. Which would then be Put() as a regular file with no links, hence adhering to the implicit --follow-link behavior podman has always had so far.

But the behavior we need is to allow podman to follow or not follow links based on a flag that is propagated from Podman to Buildah via NoDerefSymlinks. Now, in the case of not following links, we'd want to pass on this item as a TypeSymlink over to Put with the same target as on the host (this is not ideal but this is what docker does and we're aiming for parity).

[1] - https://github.com/danishprakash/buildah/blob/b5060e29537ec0283a948f59148a07f0c83d4e92/copier/copier.go#L1160

A friendly reminder that this PR had no activity for 30 days.

@nalind PTAL

A friendly reminder that this PR had no activity for 30 days.